-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
truncate: eliminate duplicate stat() syscall #9527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
truncate: eliminate duplicate stat() syscall #9527
Conversation
| /// assert_eq!(mode.to_size(fsize), None); | ||
| /// ``` | ||
| fn to_size(&self, fsize: u64) -> u64 { | ||
| fn to_size(&self, fsize: u64) -> Option<u64> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, this method could panic if rounding to 0. Now, the method returns None if rounding to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to add a unit test for this? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already added. See lines 359:361.
| Self::RoundDown(size) => fsize.checked_rem(*size).map(|remainder| fsize - remainder), | ||
| Self::RoundUp(size) => fsize.checked_rem(*size).map(|remainder| fsize + remainder), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As previously stated, calling this method when rounding to 0 resulted in a panic. Following the guideline of avoiding panicking code, use checked_rem() method.
| fn is_absolute(&self) -> bool { | ||
| matches!(self, Self::Absolute(_)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny helper function to determine if self is absolute. Useful when checking if an absolute mode was provided with a reference file.
| #[cfg(unix)] | ||
| if let Ok(metadata) = metadata(path) { | ||
| if metadata.file_type().is_fifo() { | ||
| return Err(USimpleError::new( | ||
| 1, | ||
| translate!("truncate-error-cannot-open-no-device", "filename" => filename.to_string_lossy().quote()), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check was moved to line 212.
| let file_size = match metadata(path) { | ||
| Ok(metadata) => { | ||
| #[cfg(unix)] | ||
| if metadata.file_type().is_fifo() { | ||
| return Err(USimpleError::new( | ||
| 1, | ||
| translate!("truncate-error-cannot-open-no-device", "filename" => filename.to_string_lossy().quote()), | ||
| )); | ||
| } | ||
| metadata.len() | ||
| } | ||
| Ok(m) => m, | ||
| Err(_) => 0, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieve the size of the file, while checking if it is a pipe. One single stat() syscall required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add this as a comment in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments.
| // Omitting the mode is equivalent to extending a file by 0 bytes. | ||
| let mode = match size_string { | ||
| Some(string) => match parse_mode_and_size(string) { | ||
| Err(error) => { | ||
| return Err(USimpleError::new( | ||
| 1, | ||
| translate!("truncate-error-invalid-number", "error" => error), | ||
| )); | ||
| } | ||
| Ok(mode) => mode, | ||
| }, | ||
| None => TruncateMode::Extend(0), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying no mode implies TruncateMode::Extend(0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, better in the code as comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a comment. Look carefully at line 263. Est-ce que j'aurais dû l'écrire en français pour qu'il soit plus clair ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oups pardon :)
| Self::Reduce(size) => { | ||
| if *size > fsize { | ||
| 0 | ||
| } else { | ||
| fsize - size | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard library provides saturating_sub(). No need to implement its logic by ourselves.
|
GNU testsuite comparison: |
Signed-off-by: Jean-Christian CÎRSTEA <[email protected]>
1. The documentation for parse_mode_and_size() has been updated. 2. Added documentation for TruncateMode::is_absolute(). Signed-off-by: Jean-Christian CÎRSTEA <[email protected]>
a5efbf3 to
abcae00
Compare
|
GNU testsuite comparison: |
When no reference file is given,
truncateperforms twostat()syscalls:This commit fixes this issue by restructuring the code. The main change resides in the elimination of the initial match performed at the top level to select between different scenarios. Instead, the new implementation:
TruncateMode::Extend(0)is implied.ftruncate()to resize the file.